Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Data: Registry - check for window #9261

Merged
merged 2 commits into from
Aug 25, 2018
Merged

Data: Registry - check for window #9261

merged 2 commits into from
Aug 25, 2018

Conversation

coderkevin
Copy link
Contributor

@coderkevin coderkevin commented Aug 23, 2018

Description

The current registry code is blindly deferencing window, which will fail if ever
run in node for test or server-side rendering. This adds a simple check
to ensure window exists before dereferencing.

How has this been tested?

There's not much to test for this line, as it only affects redux dev tools when there is no global window instance, which should never happen.

Types of changes

Sort of a bug fix maybe? As this code is causing a CI test to fail for Calypso: Automattic/wp-calypso#26838

This change should not break a thing, as it's just doing an additional check.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.

The registry code is blindly deferencing window, which will fail if ever
run in node for test or server-side rendering. This adds a simple check
to ensure window exists before dereferencing.
@@ -140,7 +140,7 @@ export function createRegistry( storeConfigs = {} ) {
*/
function registerReducer( reducerKey, reducer ) {
const enhancers = [];
if ( window.__REDUX_DEVTOOLS_EXTENSION__ ) {
if ( window && window.__REDUX_DEVTOOLS_EXTENSION__ ) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this check should be typeof window !== 'undefined' && -- otherwise, Automattic/wp-calypso#26838 won't even start for me but fail with a ReferenceError: window is not defined.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, will update.

@gziolo gziolo added the Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) label Aug 25, 2018
@gziolo gziolo added this to the 3.7 milestone Aug 25, 2018
Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 it should help React Native, too.

@gziolo gziolo merged commit 7db0880 into WordPress:master Aug 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants